Skip to content

Adding support of map_top_n_values to Presto#18607

Merged
kaikalur merged 1 commit intoprestodb:masterfrom
jainavi17:map_top_n_values_2
Nov 16, 2022
Merged

Adding support of map_top_n_values to Presto#18607
kaikalur merged 1 commit intoprestodb:masterfrom
jainavi17:map_top_n_values_2

Conversation

@jainavi17
Copy link
Contributor

@jainavi17 jainavi17 commented Nov 2, 2022 •
edited
Loading

Add an UDF for getting top N values of a map, which returns an array of the top N values of map. Optionally a lambda comparator can also be passed to perform a custom comparison of the values. Returns all the value if the value N is greater than or equal to size of the map. N must be >= 0. For N = 0, the function returns empty array.

map_top_n_values(map(K, V), N) -> array(V)

  • For N > 0, returns the top N values of a map. If N happens to larger than size of the map, it returns all the values of the map in descending order
  • For N = 0, return empty array
  • For N < 0, function throws an error.

map_top_n_values(map(K, V), N, function(V, V, int)) -> array(V)
Return the top N values of the map sorted using the lambda comparator.

Test plan - (Please fill in how you tested your changes)
Added unit tests.
Build successfully using the following terminal command

  • ./mvnw clean install -Dtest=TestMapTopValuesFunction -fn -pl presto-main
  • ./mvnw clean install -Dtest=TestMapTopValuesComparatorFunction -fn -pl presto-main
== RELEASE NOTES ==

General Changes
* Add :func:`map_top_n_values` to return top N values of the provided map. An optional lambda comparator can also be passed as parameter for custom sorting of the values.

@jainavi17 jainavi17 requested a review from a team as a code owner November 2, 2022 13:20
@jainavi17 jainavi17 requested a review from presto-oss November 2, 2022 13:20
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 2, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jainavi17 / name: Avinash Jain (be3de6664d30abc21af36be628b9ed16189f76d8)

@jainavi17 jainavi17 changed the title Adding support of map_top_n_keys to Presto Adding support of map_top_n_values to Presto Nov 2, 2022
@kaikalur
Copy link
Contributor

kaikalur commented Nov 2, 2022

See my comments in the other one - MAP_TOP_N_KEYS - same applies here - just do it as a SQL udf

@jainavi17 jainavi17 force-pushed the map_top_n_values_2 branch 3 times, most recently from 50669cd to be3de66 Compare November 3, 2022 23:48
@jainavi17
Copy link
Contributor Author

jainavi17 commented Nov 4, 2022

See my comments in the other one - MAP_TOP_N_KEYS - same applies here - just do it as a SQL udf

Cool! Let me do this a SQL UDFs

@jainavi17
Copy link
Contributor Author

jainavi17 commented Nov 8, 2022

Hi @kaikalur , this PR is similar to #18625 . I've pretty much taken all the feedback from previous PR into consideration. So this should be quick. Let me know

@kaikalur kaikalur requested a review from highker November 10, 2022 15:57
@kaikalur
Copy link
Contributor

kaikalur commented Nov 10, 2022

Also squash the commits

@highker
Copy link

highker commented Nov 11, 2022

Same comment as the other PR..... The release note is too verbose. We only need one (long) line to explain what it does in English. Also, we need :func: annotation for the function name so it can be linked to the doc you wrote.

highker
highker approved these changes Nov 11, 2022
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash the commits

@jainavi17
Copy link
Contributor Author

jainavi17 commented Nov 11, 2022

Same comment as the other PR..... The release note is too verbose. We only need one (long) line to explain what it does in English. Also, we need :func: annotation for the function name so it can be linked to the doc you wrote.

Done!

@jainavi17
Copy link
Contributor Author

jainavi17 commented Nov 14, 2022

Can we merge this PR?

@kaikalur kaikalur merged commit ba08fac into prestodb:master Nov 16, 2022
@wanglinsong wanglinsong mentioned this pull request Jan 12, 2023
Merged
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaikalur kaikalur kaikalur approved these changes

@highker highker highker approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jainavi17 @kaikalur @highker